Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow non octal decimal escape before use strict #12366

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Nov 17, 2020

Q                       A
Fixed Issues? Babel does not throw for () => { "\8";"\9";"use strict"; } (REPL)
Tests Added + Pass? Yes
License MIT

This PR also refactors how we track numeric escape (though named as octalPositions). Currently this.state.octalPositions is reset whenever this.nextToken is called, this is unnecessary and can be avoided by only reset when a) directives are allowed in blocks and b) we have not yet seen a non-directive parsed. I renamed octalPositions to strictErrors since we need to track two errors in this PR

  • StrictOctalLiteral
  • StrictNumericEscape

However I think we should throw StrictNumericEscape for both \8 and \01, since the latter is not an octal literal but an octal escape sequence, which falls into numeric escape. I have update the error message in this PR.

This PR also avoids reparsing the numeric/string literal after strict directive in setState, since the octal error is now tracked in strictErrors.

Last this PR extracts legacy octal literal test cases from string escape sequence tests.

@kaicataldo Feedbacks are welcome since this PR is a follow-up to & inspired from #11188.

@JLHwung JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories pkg: parser labels Nov 17, 2020
@babel-bot
Copy link
Collaborator

babel-bot commented Nov 17, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/33964/

@@ -1 +1,2 @@

language/literals/string/legacy-non-octal-escape-sequence-8-strict-explicit-pragma.js(default)
language/literals/string/legacy-non-octal-escape-sequence-9-strict-explicit-pragma.js(default)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bug is caught by these two failing test262 tests.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 17, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 22c4c17:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@JLHwung JLHwung marked this pull request as draft November 17, 2020 02:35
@JLHwung JLHwung force-pushed the disallow-non-octal-decimal-escape-before-use-strict branch from 2ac4617 to ae30cba Compare November 17, 2020 16:19
@JLHwung JLHwung marked this pull request as ready for review November 17, 2020 16:19
"use strict";
04;
05;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +146 to +147
// todo(JLHwung): set strictErrors to null and avoid recording string errors
// after a non-directive is parsed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to do this here or in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer a separate PR since alternatively, todo is pronounced as never-do. 😛
Legacy octal or numeric escapes are discouraged, I feel like it is okay to leave it as-is.

@JLHwung JLHwung force-pushed the disallow-non-octal-decimal-escape-before-use-strict branch from ae30cba to 22c4c17 Compare December 3, 2020 15:47
@JLHwung JLHwung requested review from existentialism and removed request for kaicataldo December 14, 2020 16:04
Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@nicolo-ribaudo nicolo-ribaudo merged commit a0c1a9a into babel:main Dec 15, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the disallow-non-octal-decimal-escape-before-use-strict branch December 15, 2020 11:24
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 17, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants